crypto: build warning fixes #10142
crypto: build warning fixes #10142
Conversation
Resolve warning in Ubuntu Linux i386 build
Resolve conversion warning on Windows
nodejs-github-bot
added
the
c++
| BN_ULONG exponent_word = BN_get_word(rsa->e); | ||
| BIO_printf(bio, "0x%lx", exponent_word); | ||
| unsigned long exponent = // NOLINT(runtime/int) | ||
| static_cast<unsigned long>(BN_get_word(rsa->e)); // NOLINT(runtime/int) |
There was a problem hiding this comment.
This is not correct per se on 32 bits architectures where long is 32 bits and BN_ULONG is a 64 bits unsigned long long.
| @@ -146,7 +149,8 @@ void ClientHelloParser::ParseExtension(const uint16_t type, | |||
| ocsp_request_ = 1; | |||
| break; | |||
| case kTLSSessionTicket: | |||
| tls_ticket_size_ = len; | |||
| // TBD POSSIBLE DATA LOSS: | |||
| tls_ticket_size_ = static_cast<uint16_t>(len & kTLSTicketSizeMask); | |||
There was a problem hiding this comment.
@indutny Is there a reason tls_ticket_size_ is of type uint16_t instead of size_t?
There was a problem hiding this comment.
That's how much bits it occupies in binary representation. I don't have any strong feelings about making it wider, though.
There was a problem hiding this comment.
Here is what I gather from a quick grep: tls_ticket_size_ stores a ticket size and tls_ticket_ stores a pointer to some ticket information but they are only used to determine a hello.has_ticket_ boolean value. I start to wonder if we could replace the tls_ticket_ and tls_ticket_size_ members with a member like has_valid_tls_ticker_ to avoid keeping more state than necessary?
|
In src/node_crypto.c
> @@ -1536,8 +1536,9 @@ static Local<Object> X509ToObject(Environment*
env, X509* cert) { String::kNormalString, mem->length)); (void)
BIO_reset(bio); - BN_ULONG exponent_word = BN_get_word(rsa->e); -
BIO_printf(bio, "0x%lx", exponent_word); + unsigned long exponent = //
NOLINT(runtime/int) + static_cast<unsigned long>(BN_get_word(rsa->e)); //
NOLINT(runtime/int)
This is not correct per se on 32 bits architectures where long is 32 bits
and BN_ULONG is a 64 bits unsigned long long.
I will look at this.
In src/node_crypto_clienthello.cc:
> @@ -146,7 +149,8 @@ void ClientHelloParser::ParseExtension(const
uint16_t type, ocsp_request_ = 1; break; case kTLSSessionTicket: -
tls_ticket_size_ = len; + // TBD POSSIBLE DATA LOSS: + tls_ticket_size_ =
static_cast<uint16_t>(len & kTLSTicketSizeMask);
@indutny Is there a reason tls_ticket_size_ is of type uint16_t instead
of size_t?
:+1: for fixing on my part.
@bnoorduis I will look at these and your comments on the other issues when
I get home tonight or maybe tomorrow morning. Thanks for the feedback!
|
|
Closing for now, I would like to deal with this later. |
Checklist
make -j8 test(UNIX [macOS]; Linux i386 & amd64) andvcbuild test nosign(.\vcbuild.bat nosignthen.\vcbuild.bat test nosign nobuildas Administrator on Windows) PASS OKAdditional item
Affected core subsystem(s)
crypto
Description of change
NOTE: This was originally a part of PR #10139 (build, warning, header, and include fixes).
ADDITIONAL NOTE: This and other changes related to PR #10139 are marked
// TBD POSSIBLE DATA LOSS:since I think we should examine these conversions at some point. I would be happy to take some or all of these out if necessary.